I just now rebased to see if we can get a working preview build.

Awesome, I'll get these comments addressed now 🚀

Coverage report

This PR does not seem to contain any modification to coverable code.

Okay, I gave this a whirl on the read the docs preview build, and for the most part it's great!

There's just one thing. I know you're already aware of this, but I find it really annoying that I can sometimes click on a link in the right sidebar table of contents and sometimes (when the corresponding heading is near bottom of the page and too close to headings above) it doesn't highlight that link in the TOC after I click it. This doesn't match the pattern established in the rest of the site, where clicking a TOC link (left bar or header nav) highlights that link. This feels like a big enough pattern break and UX frustration to be a blocker. I think without this being fixed I would prefer merging my PR over this one, even if it removes scroll spying, simply because it gets the TOC link highlighting correct. What do you think? Is this something you want to fix? Would you like me to look into fixing it?

Agreed, but what's the right way to handle this? I think with the current machinery, as the page scrolls down to the selected heading it will highlight the TOC elements you see as the viewport scrolls down. I can think of a few ways of doing this with unobserve/observe, but I think I'd start by trying a few methods and seeing what works.

Ahh, I don't think we can merge this yet. Tests are not working for me locally, and I think CI is not running the playwright tests, looking through the logs. Here's the invocation from CI.yml:

      - name: "Run tests ✅"
        shell: bash
        run: |
          # this will compile the assets and translations then run the tests
          # check if there is a specific Sphinx version to test with
          # example substitution: tox run -e compile-assets,i18n-compile,py39-sphinx61-tests
          if [ -n "${{matrix.sphinx-version}}" ]; then
            python -Im tox run -e compile-assets,i18n-compile,py$(echo ${{ matrix.python-version }} | tr -d .)-sphinx$(echo ${{ matrix.sphinx-version }} | tr -d .)-tests
          # if not we use the default version
          # example substitution: tox run -e compile-assets,i18n-compile,py39-tests
          else
            python -Im tox run -e compile-assets,i18n-compile,py$(echo ${{ matrix.python-version }} | tr -d .)-tests
          fi

So CI runs for example the py312-sphinx61-tests tox command, which invokes coverage run -m pytest -m "not a11y". In principle this should run everything in tests/test_playwright.py, but at the top of that file there's an playwright = pytest.importorskip("playwright") - which I think means we are skipping all playwright tests, since playwright is not a dependency (except in the a11y-tests environment, which we aren't using here). Can someone confirm?

I think CI is not running the playwright tests

That sounds right to me, based on what you wrote. I think that's probably why a number of tests like this have been snuck into the test_a11y file, even though they are not super specific to accessibility. You could take the same approach. I don't think you really need a test fixture for your test unless you were trying to be more fine grained about what you're testing. But if your test is just the following two parts:

1. Scroll to first heading, check that the first TOC entry is highlighted
2. Scroll to bottom of page, check that the first TOC entry is not highlighted

then I think you can reuse one of the docs page built for test_a11y

So CI runs for example the py312-sphinx61-tests tox command, which invokes coverage run -m pytest -m "not a11y". In principle this should run everything in tests/test_playwright.py, but at the top of that file there's an playwright = pytest.importorskip("playwright") - which I think means we are skipping all playwright tests, since playwright is not a dependency (except in the a11y-tests environment, which we aren't using here). Can someone confirm?

🤦🏻 sorry this was probably my oversight in #1861

@peytondmurray - I think this is now ready. Could you review my latest commits, update the PR description and take this out of draft mode?

@drammock - Once this pull request is merged, there should be a good period of time where this feature gets tested across a variety of people's sites and machines before being released in the next version of PyData Sphinx Theme.

Uncovered edge case here: https://pydata-sphinx-theme--2119.org.readthedocs.build/en/2119/examples/kitchen-sink/structure.html

First link in the TOC has href="#"